Fix recursive search in Client.get_items #799
Conversation
pystac_client/client.py
Outdated
| except APIError: | ||
| child_catalogs = [catalog for catalog, _, _ in self.walk()] | ||
| search = self.search(ids=ids, collections=[self, *child_catalogs]) |
There was a problem hiding this comment.
This seems like it would be pretty easy to do accidentally. I think I'd prefer to just let the error raise and make it a little harder to get every single item in planetary computer for instance.
There was a problem hiding this comment.
My concern is that without something like this, many functions that call get_items simply don't work for planetary computer or similar APIs that enforce this required argument. This includes:
Client.get_all_items,Client.walk,Client.validate_all,Client.describe,Client.make_all_asset_hrefs_relative,Client.make_all_asset_hrefs_absolute
Note that the spec doesn't say one way or another that these arguments must be optional so I'm guessing that planetary computer's API is still spec compliant technically. However, the examples show that a search without collections should be supported so I don't really know one way or the other how to interpret that:
Otherwise the only way to make this work for APIs like planetary computer is to override the Client class like:
import pystac_client
class Client(pystac_client.Client):
def search(self, *args, **kwargs):
if kwargs["collections"] is None:
kwargs["collections"] = [self.id *[catalog.id for catalog, _, _ in self.walk()]]
return super().search(*args, **kwargs)
pystac_client.client.Client = Client # so that sub-catalogs also use the updated search method If that's the approach we want to go with that's fine, but maybe we should document this workaround in case users want to interact with planetary computer.
What do you think?
There was a problem hiding this comment.
Thank you for taking the time to write that all up! I think as long as a clear error surfaces it is fine to have those methods fail on Planetary Computer. Requiring collections is not technically compliant with the spec, so I think it is better to not bake in special handling for this scenario especially since it is likely to result in a surprising user experience (setting collections to include every collection might be very very slow).
Co-authored-by: Julia Signell <[email protected]>
Co-authored-by: Julia Signell <[email protected]>
|
@jsignell I realize that the default value for This is necessary because all of the For example: if you have a catalog which contains subcatalogs, then I would like to make the default |
What if we just leave the default as None? That way this PR is purely additive -- it adds the option to set |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #799 +/- ##
==========================================
+ Coverage 93.43% 94.22% +0.79%
==========================================
Files 13 15 +2
Lines 990 1213 +223
==========================================
+ Hits 925 1143 +218
- Misses 65 70 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
If we leave the default as Either way, it changes the semantics of |
Sorry why not just let None be treated as True? |
OK here's an example:
A concrete example: Let's say you have the following structure:
|
|
Thank you for spelling it out for me. This inheritance model is not pretty. I think I feel ok with changing the default to recursive=False as long as we use recursive=True in |
Related Issue(s):
Client.get_itemshas surprising recursive behaviour when using the/searchendpoint #798Description:
FalsePR Checklist: